Fix inline-linear-SCC topsort cycle when chaining Euler orientation states (MTK#4608)#113
Closed
baggepinnen wants to merge 1 commit into
Closed
Conversation
…ce (#98) Resolves the `topsort_equations` "equations have at least one cycle" failure when chaining >=2 rotation-matrix (Euler) orientation states (SciML/ModelingToolkit.jl#4608). `get_linear_scc_linsol` gated coefficient extraction on the structural incidence `graph` (`has_edge`). That graph is built before `total_sub` substitution and is mutated during reassembly, so it can desync from the substituted residual: when `total_sub` reintroduces an SCC variable (e.g. a chained joint inherits the upstream angular velocity, pulling the Euler-angle rate into a residual whose structural edge was already torn), the gate skips expanding that variable into the coefficient matrix `A` and leaves it buried in `b`. `__reduce_linear_system!` then smuggles it onto the RHS, producing a self-referential `A \ b` whose observed equations cannot be topologically ordered. Gate extraction on the actual residual incidence instead of the stale graph, so every present SCC variable is expanded into `A` and `b` is free of SCC variables by construction -- making the opt-in `_assert_b_free_of_scc_vars` invariant (added in #106) hold rather than merely checking it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
|
Superseded by #114 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the
mtkcompile/structural_simplifyfailureArgumentError: The equations have at least one cycle.(fromtopsort_equations) when chaining two or more rotation-matrix (Euler) orientation states — SciML/ModelingToolkit.jl#4608.Stacks on #106, which adds the opt-in
_assert_b_free_of_scc_varsinvariant. This PR makes that invariant hold by construction (and thus actually fixes #4608), rather than only checking it underMTKTEARING_CHECK_REDUCTION.Root cause
get_linear_scc_linsolbuilds the inlineA \ bblock by extracting each SCC variable's coefficient out of every (total_sub-substituted) residual, gated on the structural incidence graph viahas_edge(graph, eq, var). That graph is built beforetotal_subsubstitution and is mutated during reassembly, so it can desync from the residual:total_subcan reintroduce an SCC variable into a residual whose structural edge was already torn. Thehas_edgegate then skips expanding that variable into the coefficient matrixA, leaving it buried inb.__reduce_linear_system!— which only inspects coefficient rows, neverb— subsequently smuggles it onto the RHS, producing a self-referentialA \ b(the solution vector appears inside its own RHS). The resulting observed equations form a cycle thattopsort_equationsrejects.Confirmed on the #4608 MWE: the buried variables are exactly the second joint's Euler rates
j2.phid[1:3], all withhas_edge == false. The world-rooted first joint stays in sync (constantR, zero angular velocity), which is why a single Euler joint compiles but two chained do not.Fix
Gate extraction on the actual residual incidence (
get_variableson the substituted residual) instead of the stale structural graph. Every SCC variable genuinely present in a residual is expanded intoA, sobis free of SCC variables by construction. Blocks whose structural graph is already in sync get identical extraction and identical output — no behavior change on healthy models.Verification (registry MTK 11.26.8)
A/B over models that expose the cycle and controls that do not:
Spherical(control)SphericalSphericalCable, 3 Euler segmentsCable, 5 Euler segmentsCable, 3 Quaternion (control)All controls keep identical unknown counts → no regression. With
MTKTEARING_CHECK_REDUCTION=1the#106self-check confirms the previously self-referentialj2.phidblock is now consistent (relres ≈ 4e-17).Scope
Draft. This fixes the structural cycle only. Runtime solving of these models hits a separate, non-rank-tolerant inline-
\(LU) issue at degenerate configurations — independent of the cycle and out of scope here.🤖 Generated with Claude Code